Skip to content

add content-filtered-topic interfaces#181

Merged
fujitatomoya merged 8 commits intoros2:masterfrom
iuhilnehc-ynos:topic-content_filtered_topic
Mar 21, 2022
Merged

add content-filtered-topic interfaces#181
fujitatomoya merged 8 commits intoros2:masterfrom
iuhilnehc-ynos:topic-content_filtered_topic

Conversation

@iuhilnehc-ynos
Copy link
Contributor

@iuhilnehc-ynos iuhilnehc-ynos commented Feb 25, 2021

Related to ros2/design#282 to add content-filtered-topic interfaces.

@iuhilnehc-ynos iuhilnehc-ynos mentioned this pull request Feb 25, 2021
Merged
15 tasks
fujitatomoya
fujitatomoya reviewed Mar 11, 2021
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with test.

i think it would be nice to add test for both interfaces, along with rmw_take.

at this moment, no implementation supports those interfaces. but once rmw_connextdds is merged, we can enable and adjust the test.

@iuhilnehc-ynos
Copy link
Contributor Author

iuhilnehc-ynos commented Mar 11, 2021

LGTM with test.

i think it would be nice to add test for both interfaces, along with rmw_take.

at this moment, no implementation supports those interfaces. but once rmw_connextdds is merged, we can enable and adjust the test.

OK, I'll add this kind of test into rcl by using a flag, such as is_connextdds or is_vendor_support_cft.

@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-content_filtered_topic branch from 79893f7 to ca64b75 Compare March 12, 2021 05:05
@iuhilnehc-ynos
Copy link
Contributor Author

iuhilnehc-ynos commented Mar 12, 2021

@fujitatomoya

I have added some test cases in rcl, if you're free, you could review ros2/rcl@be5a640

@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-content_filtered_topic branch from ca64b75 to 671a886 Compare October 26, 2021 07:16
@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-content_filtered_topic branch from fa04ac8 to 0c8c819 Compare January 13, 2022 05:46
@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Mar 2, 2022

@audrow @mjcarroll
CC: @ivanpauno @wjwwood

Could you review this PR. (asking cz i see your names as maintainer.)

@ivanpauno
Copy link
Member

ivanpauno commented Mar 2, 2022

Could you review this PR. (asking cz i see your names as maintainer.)

I'm planning to continue reviewing the PRs after I get concensus with the team about the plan mentioned in ros2/rmw#302 (comment).
I have already added the topic for discussion in a meeting next Tuesday.

@fujitatomoya fujitatomoya mentioned this pull request Mar 17, 2022
Merged
Chen Lihui added 7 commits March 18, 2022 11:05
Add functions for supporting a feature of content filtered topic
e3f6142 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
to support reset content filtered topic with empty string.
1433137 
remove constness for rmw_subscription because is_cft_supported might be updated

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Update interface
5dcf45a 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
rename
af46b95 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
add test cases
bec59fd 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
add another test
549091f 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
relate to rcutils_string_array_t expression_parameters changed in rmw
f780a03 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@iuhilnehc-ynos iuhilnehc-ynos force-pushed the topic-content_filtered_topic branch from ca5e920 to f780a03 Compare March 18, 2022 03:48
asorbini
asorbini reviewed Mar 19, 2022
View reviewed changes
Copy link
Contributor

@asorbini asorbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failures with rmw_connextdds should be resolved after adding a short sleep when changing the content filter.

wait to allow for filter propagation
5739549 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Mar 21, 2022

CI is green, i will merge this into mainline.

@fujitatomoya fujitatomoya merged commit 9baef10 into ros2:master Mar 21, 2022
@fujitatomoya fujitatomoya mentioned this pull request Mar 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

+1 more reviewer

@asorbini asorbini asorbini left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ivanpauno ivanpauno

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iuhilnehc-ynos @fujitatomoya @ivanpauno @asorbini